Fix three bugs in step_manager (missing return, wrong error text, header array shape) - #28
Closed
benjithompson wants to merge 3 commits into
Closed
benjithompson wants to merge 3 commits into
benjithompson wants to merge 3 commits into
Conversation
An unsupported body_type constructed the error BaseResult but did not return it, so execution fell through and PUT the step with an empty body and no Content-Type header. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S2Ym1SshabyXjWNdeGFDfx
The non-request-step guard reported "cannot have a body added" when rejecting an assertion. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S2Ym1SshabyXjWNdeGFDfx
The REST API documents header values as arrays of strings
(e.g. {"Content-Type": ["application/json"]}); bare strings relied
on undocumented server leniency.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S2Ym1SshabyXjWNdeGFDfx
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
There was a problem hiding this comment.
Pull request overview
This PR tightens StepManager request-step mutation behavior in src/tools/step_manager.py and adds regression coverage in tests/test_step_manager.py, addressing validation fall-through, a misleading error message, and request header payload shape to better match the documented REST contract.
Changes:
- Fixes a missing
returnso unsupportedbody_typeerrors short-circuit before any API calls. - Corrects a copy-pasted error message when adding assertions to non-request steps.
- Changes emitted
Content-Typeheader values to arrays of strings (e.g.["application/json"]) when adding a body.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/tools/step_manager.py |
Returns early on unsupported body_type, fixes assertion guard error text, and emits Content-Type header values as arrays. |
tests/test_step_manager.py |
Adds regression tests for unsupported body_type, non-request assertion guard messaging, and verifies Content-Type header array shape (JSON case). |
Comments suppressed due to low confidence (1)
src/tools/step_manager.py:161
- The register() tool description for add_assertion_to_step claims assertions can be added to non-request step types (Ghost Inspector/subtest/conditional), but the implementation hard-rejects anything except step_type == 'request'. Please either broaden the implementation to support the documented step types, or update the description to reflect the actual restriction.
request_result = await self.read(bucket_key, test_id, step_id, result_formatter=None)
if not request_result or request_result.get("step_type") != "request":
return BaseResult(
error=f"Step {step_id} is not a request step and cannot have an assertion added."
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| except json.JSONDecodeError as e: | ||
| return BaseResult(error=f"Invalid JSON content provided for body_content: {str(e)}") | ||
| request_headers["Content-Type"] = "application/json" | ||
| request_headers["Content-Type"] = ["application/json"] |
Comment on lines
93
to
+104
| @@ -101,7 +101,7 @@ async def add_body_to_step( | |||
| return BaseResult(error=f"Invalid XML content provided for body_content: {str(e)}") | |||
| except Exception as e: | |||
| return BaseResult(error=f"Error processing XML content: {str(e)}") | |||
| request_headers["Content-Type"] = "application/xml" | |||
| request_headers["Content-Type"] = ["application/xml"] | |||
pjain05
approved these changes
Jul 24, 2026
Collaborator
|
@copilot resolve the merge conflicts in this pull request |
4 tasks
Collaborator
|
Since this PR was from a forked repo, creating a similar PR #33 and closing this one. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three small, independent fixes to
StepManager(src/tools/step_manager.py), each with aregression test.
1. Missing
returnfor unsupportedbody_typeinadd_body_to_stepThe unsupported-
body_typearm constructed aBaseResult(error=...)but never returned it, soexecution fell through and PUT the step with an empty body and no
Content-Typeheader instead ofsurfacing the validation error. Added the
return.2. Copy-pasted error message in
add_assertion_to_stepThe non-request-step guard reported "...cannot have a body added" when rejecting an assertion.
Corrected the message to refer to an assertion.
3. Emit
Content-Typeheader values as arraysThe REST API documents header values as arrays of strings
(e.g.
{"Content-Type": ["application/json"]}); the code sent bare strings and relied onundocumented server leniency. Now emits arrays for all body types (json/xml/html/text).
Testing
pytest tests/test_step_manager.py— 11 passed. flake8 / black / isort clean.